Skip to content

Conversation

@akshayutture-augment
Copy link

@akshayutture-augment akshayutture-augment commented Nov 17, 2025

No description provided.

* remove the use of client side cache for in-proc authz client

Co-authored-by: Gabriel MABILLE <[email protected]>

* add a permission denial cache, fetch perms if not in either of the caches

Co-authored-by: Gabriel MABILLE <[email protected]>

* Clean up tests

Co-authored-by: Ieva <[email protected]>

* Cache tests

Co-authored-by: Ieva <[email protected]>

* Add test to list + cache

Co-authored-by: Ieva <[email protected]>

* Add outdated cache test

Co-authored-by: Ieva <[email protected]>

* Re-organize metrics

Co-authored-by: Ieva <[email protected]>

---------

Co-authored-by: Gabriel MABILLE <[email protected]>
Copy link

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

return &authzv1.CheckResponse{Allowed: allowed}, nil
}
}
s.metrics.permissionCacheUsage.WithLabelValues("false", checkReq.Action).Inc()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This increments permissionCacheUsage with "false" even when getCachedIdentityPermissions was a cache hit but allowed == false, which misclassifies a cache hit as a miss. Consider counting a cache hit whenever err == nil (regardless of allow/deny), or only incrementing the "false" metric when the cache lookup actually missed.

🤖 Was this useful? React with 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants